Support for initial entries #432
Conversation
should be specified in the P4Runtime spec.
|
As of commit 2 of this PR, this is a first draft of a proposal for addressing issue #426 There are still several embedded questions marked TODO at this point. Please comment with suggested answers to those questions if you have any. |
proto/p4/v1/p4runtime.proto
Outdated
| // P4 source code within its `entries` table property. | ||
| // | ||
| // TODO: Should const be true for all entries read from a table | ||
| // declared with `const entries`? Would that be considered a |
There was a problem hiding this comment.
Existing implementations will not populate this field, evaluating to 0=false, even for constant entries, resulting in bogus information. I don't know if this constitutes "backwards incompatibility," or it simply means existing implementations are suddently non-conformant with a new requirement. If the consensus is to adopt the proposal despite this fact, we would need at a minimum to write an advisory that this field should be ignored unless the implementation conforms with the spec version which adopts this.
There was a problem hiding this comment.
That does seem like an undesirable state. One way to avoid such a situation is to define that when reading entries from a table with const entries, or from a default entry declared with const, that this new TableEntry const flag will always be false, to ensure that old and new servers return the same result for these kinds of read requests both before and after this spec change is made.
If we did that, we would try to document this as clearly as we can, and the reason why (because it would not be immediately obvious to someone new to the P4Runtime spec a year or two from now), and that the only time the server would set the const flag to true in a response to a read request would be for a table that had a const flag on an entry in a table with this new style of initial entries.
There was a problem hiding this comment.
Your logic is sound, but it doesn't feel satisfying or intuitive overall - too many conditions to wrap one's head around. It seems like a long-term solution to a short-term problem. I'm not sure how objectionable it is - clients of this older server could simply not try to use the field until the server supports it, same as any other new feature. Perhaps we should get others to weigh in on this.
There was a problem hiding this comment.
I would love to get others to weigh in on this. @smolkaj @jonathan-dilorenzo @thantry Can you or someone you know at Google who is interested in getting initial entries actually working in an implementation (as opposed to merely specified in P4.org documents) please take a look, or get someone else to take a look, at this PR in detail and comment?
There was a problem hiding this comment.
I would still like others to weigh in on this question, but thinking about this some more, it does seem in the spirit of Protobuf based messages to consider it still 'backwards compatible' if an old message has a new field with additional information added to it. If we have a new const field in TableEntry messages, and new P4Runtime servers in the future return true for this field for entries declared with const entries in the source code, even though older P4Runtime servers return no such field at all, that seems like something that P4Runtime API clients should just need to handle gracefully.
There was a problem hiding this comment.
With commit 12 on this PR, I have changed the proposed comment being commented on here to say that this new const field for TableEntry messages WILL be set to true by the server for all entries read from a table with const entries, and also for reading the default action from a table that has const default_action. We can change that before approving, of course, depending on whether anyone else has arguments against this choice.
There was a problem hiding this comment.
Sorry for missing this until now.
I am not someone who is planning to immediately use initial entries at this point, but can answer from a protobuf / semantic versioning perspective.
This requires a MINOR version bump in terms of https://semver.org/.
This is a "backward compatible API change" in the sense that an "old client" (using the old .proto) will continue to work without problems with a "new server" who adopts the new API:
- READ: When an old client gets a message with the new field (in binary format), protobuf will simply ignore the extra data and the client will not see it.
- WRITE: When an old client writes a message without the new field (in binary format), a new server will see the field as being set to
false. That works out fine for this proposal.
Note that a new client won't automagically work with an old server, as mentioned by other above: they will read back all entries with is_const: false even if the entries are const. But since the client made a deliberate choice here to adopt the new API, it's also their job to ensure they can support older servers as well, if they wish. ("This is a backward compatible API change" means that upgrading servers to the new API won't break clients -- it doesn't talk about what happens if clients start using the new API with servers that don't support it...)
@jafingerhut Perhaps we should leave a comment saying that the field will be set by P4Runtime servers starting at version x.y.z?
There was a problem hiding this comment.
Version x.y.z of what project? Stratum? p4lang/PI? The P4Runtime API specification?
There was a problem hiding this comment.
Servers implementing version x.y.z of the P4Runtime standard.
I think it's probably enough to say something like "Added in 1.4.0"?
EDIT: I also opened #441, I think we should do this consistently going forward.
There was a problem hiding this comment.
I added two "Added in 1.4.0" comments with my latest commit to this PR, one for is_const in TableEntry, and the other for has_initial_entries in Table.
and clean up Madoko formatting in that appendix.
and also for const default_action
|
Jonathan DiLorenzo: Should we have tables with Andy: I will make this change in another commit on this PR and let people know it is ready for review. |
Also fix a couple of spelling typos.
|
I have updated this PR to follow the suggestion made during the 2023-Jul-14 API work group meeting, so that tables declared in source code with The new style of tables that the language spec now supports that can have |
docs/v1/P4Runtime-Spec.mdk
Outdated
| of tables into the device. | ||
|
|
||
| The format of the entries file is a sequence of 0 or more `Update` | ||
| messages. All of them have `type` equal to `INSERT`, and `entity` is |
There was a problem hiding this comment.
I suspect just a WriteRequest?
The protobuf parsers don't really support sequences of messages out of the box AFAIK, so if it was really just a concatenation of parsable protobuf blobs that would make things pretty hard to consume.
There was a problem hiding this comment.
Here is an example entries file written by p4c in text format: https://github.com/p4lang/p4c/blob/main/testdata/p4_16_samples_outputs/table-entries-ternary-bmv2.p4.entries.txt
It doesn't look like a WriteRequest message to me, since there are no fields of a WriteRequest message present there, only a sequence of 0 or more Update messages. If you tell me that is a valid WriteRequest message, I am happy to document it that way in the P4Runtime spec.
There was a problem hiding this comment.
Yup, the link you provided does show a valid WriteRequest.
It doesn't look like a WriteRequest message to me, since there are no fields of a WriteRequest message present there
Actually it does: the repeated updates field is a WriteRequest field [1], and the textproto you linked to contains a bunch of updates { ... }.
This is admittedly subtle, because the text proto format doesn't include an outer wrapper for the message that is being encoded. Instead, it just shows the fields of the message at the top-level.
That being said, the text format is ambiguous, this could be a dump of any proto message with a repeated Update field. And for the binary format, this distinction matters. So we shouldn't blindly claim this is a WriteRequest. Do you happen to know where in the p4c source code this output gets created? From there it will be straightforward to tell the correct message type.
There was a problem hiding this comment.
Here is the p4c code that writes the entries file: https://github.com/p4lang/p4c/blob/main/control-plane/p4RuntimeSerializer.cpp#L973-L976
in case the line numbers go askew, the method name is P4RuntimeEntriesConverter
There was a problem hiding this comment.
If you would like to examing the binary and/or JSON format of the entries file generated by current open source p4c, in addition to the text format, they are all in the zip file attached to this comment.
tmp.zip
There was a problem hiding this comment.
I didn't find the place where things get serialized, but I would consider the following a good-enough hint that the message is a p4.v1.WriteRequest: https://github.com/p4lang/p4c/blob/main/control-plane/p4RuntimeSerializer.cpp#L1327-L1328
I also opened p4lang/p4c#4070 so people don't have to guess in the future.
There was a problem hiding this comment.
Is there an easy way for you to parse the contents of the binary format entries file in the file tmp.zip attached in my previous comment, to determine whether it is a valid binary WriteRequest protobuf message? I suspect it probably is, but do not know any quick way to check.
There was a problem hiding this comment.
Confirmed with a Google-internal CLI tool that entries.bin can be parsed as a p4.v1.WriteRequest.
Propose that it is true if and only if the table property `entries` is present, _and_ the list of entries is not empty.
|
@smolkaj @jonathan-dilorenzo @chrispsommers I have made 7 commits to this PR since the 2023-Jul-14 API work group meeting, primarily due to comments made during the meeting, and from Steffen's careful review after the meeting, and I think it is the better for it. I have no current plans to change it any more, until and unless there is additional review feedback that warrants additional changes. |
|
This LGTM! There seem to be 3 remaining threads that would be nice to resolve, 2 of them being nits. Other than that, from my side this is ready to be merged. |
With commit 22 on this PR, I have attempted to clarify the description of the protobuf schema of an entries file. |
|
Looks great to me. |
|
@saynb @jamescchoi This has been through careful review by several others already, so hopefully should also meet with your approval, but in case you want to take a look, this is something we are going to be asked to implement by some folks, so good to be aware of the details. |
These changes update the contents of P4Info and table entries files as generated by p4c, to match what has been agreed upon by the P4 API work group in this pull request p4lang/p4runtime#432
| in the source code, and thus `is_const_table` and | ||
| `has_initial_entries` will both be false. A corner case is that if | ||
| it has `entries = { }` with no `const` before `entries`, &ie; an | ||
| empty list of entries, that is also a normal table. |
There was a problem hiding this comment.
It occurs to me that handling this corner-case gracefully may be useful for cases where source code is dynamically-generated based on e.g. macros or other techniques. In such cases, certain "build profiles" may result in an empty entries list.
docs/v1/P4Runtime-Spec.mdk
Outdated
| Note that for a table with `const entries`, or with `entries` having | ||
| one or more individual entries declared `const`, it would be an error | ||
| for a P4Runtime client to send a `WriteRequest` to a P4Runtime server | ||
| with the contents of the entries file, because the server should |
There was a problem hiding this comment.
It may be good to further clarify that error applies to the entries with const only, not the whole contents of the entries file.
There was a problem hiding this comment.
With the latest commit on this PR as of writing this comment, I have replaced the last paragraph of the new appendix with the following, in an attempt to clarify:
Note that if a P4Runtime client attempted to send a `WriteRequest` to
a P4Runtime server with the contents of the entries file, the server
must return an error for each entry that has `is_const` true, as
described in Section [#sec-table-entry].
|
@smolkaj @chrispsommers Happy to discuss this at the next P4 API work group meeting on 2023-Aug-11, in hopes of seeing if it has the right approvals for merging. |
|
We have 4 approvals, so I think we are good to go. |
* Continuing the implementation of initial entries support in p4c These changes update the contents of P4Info and table entries files as generated by p4c, to match what has been agreed upon by the P4 API work group in this pull request p4lang/p4runtime#432 * Comment-only change to poke CI. * Update p4runtime commit SHA * Update p4runtime commit SHA for Bazel builds * Correctly handle case when table->getEntries() is null * Update a few more expected output files, and make linter happy * Update bazel/p4c_deps.bzl Co-authored-by: Radostin Stoyanov <[email protected]> --------- Co-authored-by: Radostin Stoyanov <[email protected]>
* Update Go dependencies (#438) * Fixes #439 (#440) Change CI workflow to skip publishing if PR spawned by dependabot * Adding dev branches to workflows (#443) * Support for initial entries (#432) * Define P4Runtime API support for tables with initial entries * Add TODO asking whether the format for the contents of entries files should be specified in the P4Runtime spec. * Fix a couple of things found by linter and compiling protobuf * Update autogenerated files * Document that TableEntry const field must be false in write requests * Add an appendix describing the contents of entries files generated by p4c * Clarify some wording. * Fix Madoko lint check * Replace TODO with cross reference to new appendix on entries files and clean up Madoko formatting in that appendix. * Replace TODO with an optimistic footnote. * Propose that TableEntry has new field const true for const entries and also for const default_action * Update auto-generated files * Define has_initial_entries to be true for tables with `const entries` Also fix a couple of spelling typos. * Update auto-generated files * Address several review comments * Address some more review comments. * Update auto-generated files again * Slight change in definition of has_initial_entries flag Propose that it is true if and only if the table property `entries` is present, _and_ the list of entries is not empty. * Update auto-generated files * Add "added in 1.4.0" notes to the two new fields * Clarify the description of the content of an entries file * Fix a typo, and add is_const field to list of TableEntry fields * Address review comment in new appendix * Fix #434: Remove obsolete TODO section in README (#447) * Fix #434: Remove obsolete TODO section in README Update the link to the auto-generated versions of the P4Runtime specification on the P4.org web site. Update the section "P4 Language Version Applicability" to version 1.2.4 of the P4_16 language specification, but list 3 known exceptions of features that have not been explicitly addressed yet. * Add P4_16 v1.2.4 language spec features that may need addressing in a future version of the P4Runtime API specification. * Update discussion of entry priorities in constant tables (#457) * Update discussion of entry priorities in constant tables * Correct description of entry priority for constant tables * Bump golang.org/x/net from 0.9.0 to 0.17.0 (#461) Bumps [golang.org/x/net](https://github.com/golang/net) from 0.9.0 to 0.17.0. - [Commits](golang/net@v0.9.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Remove 4 P4 language spec compatibility issues from the list (#459) * Remove 4 P4 language spec compatibility issues from the list During 2023-Sep-08 P4.org API work group meeting, it was agreed that there are no changes required to the P4Runtime API specification to be compatible with these updates in the language spec. * Add clarifying behavior of table with no `key` property back in since there are potentially open issues around p4c implementation and how it generates size field of tables in P4Info files that should be considered before considering that issue resolved. * Add metadata to multicast group entry (#446) Same role as the metadata field for table entry * Add proto_build_test rule that tests building the protos defined in the workspace. (#460) * Update license kind to Apache2.0 instead of generic notice (#464) * Bump google.golang.org/grpc from 1.56.1 to 1.56.3 (#465) Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.56.1 to 1.56.3. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.56.1...v1.56.3) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Antonin Bas <[email protected]> Co-authored-by: Chris Sommers <[email protected]> Co-authored-by: Andy Fingerhut <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: verios-google <[email protected]> Co-authored-by: anksaiki <[email protected]> Co-authored-by: anksaiki <[email protected]>
* Update Go dependencies (#438) * Fixes #439 (#440) Change CI workflow to skip publishing if PR spawned by dependabot * Adding dev branches to workflows (#443) * Support for initial entries (#432) * Define P4Runtime API support for tables with initial entries * Add TODO asking whether the format for the contents of entries files should be specified in the P4Runtime spec. * Fix a couple of things found by linter and compiling protobuf * Update autogenerated files * Document that TableEntry const field must be false in write requests * Add an appendix describing the contents of entries files generated by p4c * Clarify some wording. * Fix Madoko lint check * Replace TODO with cross reference to new appendix on entries files and clean up Madoko formatting in that appendix. * Replace TODO with an optimistic footnote. * Propose that TableEntry has new field const true for const entries and also for const default_action * Update auto-generated files * Define has_initial_entries to be true for tables with `const entries` Also fix a couple of spelling typos. * Update auto-generated files * Address several review comments * Address some more review comments. * Update auto-generated files again * Slight change in definition of has_initial_entries flag Propose that it is true if and only if the table property `entries` is present, _and_ the list of entries is not empty. * Update auto-generated files * Add "added in 1.4.0" notes to the two new fields * Clarify the description of the content of an entries file * Fix a typo, and add is_const field to list of TableEntry fields * Address review comment in new appendix * Fix #434: Remove obsolete TODO section in README (#447) * Fix #434: Remove obsolete TODO section in README Update the link to the auto-generated versions of the P4Runtime specification on the P4.org web site. Update the section "P4 Language Version Applicability" to version 1.2.4 of the P4_16 language specification, but list 3 known exceptions of features that have not been explicitly addressed yet. * Add P4_16 v1.2.4 language spec features that may need addressing in a future version of the P4Runtime API specification. * Update discussion of entry priorities in constant tables (#457) * Update discussion of entry priorities in constant tables * Correct description of entry priority for constant tables * Bump golang.org/x/net from 0.9.0 to 0.17.0 (#461) Bumps [golang.org/x/net](https://github.com/golang/net) from 0.9.0 to 0.17.0. - [Commits](golang/net@v0.9.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Remove 4 P4 language spec compatibility issues from the list (#459) * Remove 4 P4 language spec compatibility issues from the list During 2023-Sep-08 P4.org API work group meeting, it was agreed that there are no changes required to the P4Runtime API specification to be compatible with these updates in the language spec. * Add clarifying behavior of table with no `key` property back in since there are potentially open issues around p4c implementation and how it generates size field of tables in P4Info files that should be considered before considering that issue resolved. * Add metadata to multicast group entry (#446) Same role as the metadata field for table entry * Add proto_build_test rule that tests building the protos defined in the workspace. (#460) * Update license kind to Apache2.0 instead of generic notice (#464) * Bump google.golang.org/grpc from 1.56.1 to 1.56.3 (#465) Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.56.1 to 1.56.3. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.56.1...v1.56.3) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Pin Bazel version to 6.4.0 to fix regression (#471) --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Antonin Bas <[email protected]> Co-authored-by: Chris Sommers <[email protected]> Co-authored-by: Andy Fingerhut <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: verios-google <[email protected]> Co-authored-by: anksaiki <[email protected]> Co-authored-by: anksaiki <[email protected]> Co-authored-by: Steffen Smolka <[email protected]>
Fixes #426